GH-50756: [C++][FlightSQL][ODBC] Fix Clang 20 compilation on macOS 26 - #50757
Conversation
HuaHuaY
changed the title
|
|
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
I think we should resolve the discussion on the original issue before proceeding here.
github-actions
Bot
added
awaiting changes
f7986d3 to
a4aa4cb
Compare
github-actions
Bot
added
awaiting change review
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
Seems reasonable. CC @alinaliBQ
github-actions
Bot
added
awaiting changes
There was a problem hiding this comment.
Pull request overview
Fixes Clang 20 (macOS 26) compilation failures in the Flight SQL ODBC driver by addressing deprecated Unicode conversion APIs, narrowing/precision warnings treated as errors, and unused-field warnings.
Changes:
- Replace deprecated
std::wstring_convert/std::codecvt_utf8usage with vendored utfcpp conversions in ODBC encoding helpers. - Resolve narrowing/precision warnings by adding explicit casts for ODBC/Arrow indices and RapidJSON length parameters.
- Remove an unused
FlightSqlConnectionprivate field and fix zero-initialization oftm.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_statement.cc | Cast result-set column indices to int for BindColumn calls. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/odbc_descriptor.cc | Fix narrowing warnings for metadata indexing and numeric radix handling; clarify TODOs. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/main.cc | Cast column index to int for GetData calls in test/demo code. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/json_converter.cc | Cast lengths to rapidjson::SizeType to avoid implicit narrowing warnings. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_result_set.cc | Cast column index to int for Arrow RecordBatch::column(int) access. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.h | Remove unused odbc_version_ member. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/flight_sql_connection.cc | Remove now-deleted odbc_version_ initializer. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/encoding.h | Replace deprecated codecvt-based conversions with utfcpp-based conversions. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/config/configuration.cc | Avoid implicit narrowing in byte-length calculations for SQLWCHAR conversion. |
| cpp/src/arrow/flight/sql/odbc/odbc_impl/accessors/timestamp_array_accessor.cc | Fix tm initialization to avoid missing-field initializer warnings. |
| cpp/src/arrow/flight/sql/odbc/odbc_api.cc | Remove unused variable; adjust diagnostic record index type. |
| // Convert from ODBC 1 based record number to internal diagnostics 0 indexed storage | ||
| const size_t record_index = static_cast<size_t>(rec_number - 1); | ||
| const auto record_index = static_cast<uint32_t>(rec_number - 1); | ||
| if (!diagnostics->HasRecord(record_index)) { | ||
| return SQL_NO_DATA; |
There was a problem hiding this comment.
- The function whose actual argument is
record_indexhas a parameter type ofuint32_t, so there is no semantic difference to the original code. - This function has already checked
rec_number < 1on line 602, so there is no need to check it again here.
alinaliBQ
left a comment
alinaliBQ
left a comment
There was a problem hiding this comment.
Could you trigger the CI for C++ Extra / ODBC debug AMD64 macOS 15-intel (pull_request) again to see if the failure can be reproduced?
It might be an existing flakey failure; just to confirm.
|
I kicked CI again. |
a4aa4cb to
200d825
Compare
github-actions
Bot
added
awaiting change review
|
The CI run that was previously re-triggered has passed. I rebased and pushed a new commit to address the comments above. |
lidavidm
left a comment
lidavidm
left a comment
There was a problem hiding this comment.
Thanks. It seems the failures are unrelated to this PR.
github-actions
Bot
added
awaiting merge
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 669c374. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
Fix Clang 20 compilation on macOS 26.
What changes are included in this PR?
Fix four kinds of compile errors mentioned in the issue GH-50756.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.